Fix project-local flags being ignored #8623

bairyn · 2022-12-03T23:46:41Z

This was originally the first of two parts to the ‘Track static vs. dynamic dependencies.’ pull request (#8461), which I've cleaned up and split into two separate PRs.

This PR fixes an issue in which configuration flags such as ‘--disable-library-vanilla’ could be silently dropped when a package was built and installed, and it also adds a test that sets up a scenario to demonstrate the issue.

(I ran the cabal-testsuite/PackageTests test suite and found ‘432 tests, 6 skipped, 0 unexpected passes, 0 unexpected fails’.)

bairyn · 2022-12-10T02:17:23Z

I just pushed amendments to this MR that update the remaining tests (along with a rebase), outside PackageTests (isolated changes: bairyn@6446120).

gbaz · 2022-12-10T05:01:11Z

Thanks for the thoroughness of this PR and tests, for being responsive to the ask to refactor the big PR, and especially for your patience in explaining it in all the details! All very much appreciated.

gbaz · 2022-12-10T05:02:51Z

I'm approving as is, but it would be great to add a changelog entry to the changelog dir as well.

I noticed that running ‘cabal install’ with two separate sets of dynamic / static build flags (e.g. one with none, and one with ‘--enable-shared --enable-executable-dynamic --disable-library-vanilla’) produced packages with the same hash, instead of different hashes. After debugging this issue I found that this command (with no explicit cabal project file) was resulting in these build configuration flags being ignored, because in ProjectPlanning.hs, the sdist was not considered a local package, so the (non-shared) local-package-only configuration was being dropped. This fix ensures that these command-line arguments properly make it through to where they belong in cases like this. Additionally, adjust the ‘style’ attribute in plan.json so that globally installed packages are designated as global even if they are local to the project. (Without this adjustment to ‘style2str’, the T5782Diamond test fails, because it looks up ‘dist-dirs’ in plan.json, where ‘dist-dirs’ is absent from the JSON.) Finally, take into account elabDynExe and configDynExe to provide GHC with ‘-dynamic’ appropriately rather than going about it with static linking.

bairyn · 2022-12-22T07:20:07Z

Thanks gbaz.

I just updated this MR with the following updates:

Rebased onto upstream, so it's more up-to-date.
Added changelog entry.
Skip the new test on Windows (I noticed the tests I added fail on Windows for some reason; I'll just skip them on Windows for now).

Mikolaj · 2022-12-22T15:15:51Z

@bairyn: was that the same failure as on OSX currently? It would be ideal to understand it in both cases.

bairyn · 2022-12-23T00:51:42Z

No, they're different. I think what happened in the OS X case is that the Grinch stole the vowels from the package names, so when the test looks for ‘basic-0.1-’ to extract the hash (as it's done in UniqueIPID), it can't find it because it isn't looking for ‘bsc-0.1-’. I just pushed an update to the hash extraction utility to also check for ‘bsc-0.1-’, so that the utility in the test works also on OS X.

I think what happened in the Windows case is that the installation I set up (with Chocolatey) and the one that the CI server uses don't support dynamic builds. I set up a VM to reproduce the scenario, and I found that just creating a simple ‘cabal init’ application with ‘--enable-shared --enable-executable-dynamic --disable-library-vanilla’ fails with an error since the dynamic files are missing for the ‘base’ package. I updated the note around ‘skipIfWindows’.

MR Updates:

Search for ‘bsc-0.1-’, so that the new test supports OS X.
Add note about skipping on Windows (needs support for dynamic builds).

Mikolaj

Thanks a lot and especially for the test.

Please set one of the merge_me labels and in 2 days it should get auto-merged.

Mikolaj · 2023-02-01T22:25:23Z

Hi there! Heads up that we most probably need to improve this PR before we can release cabal 3.10. The main issue is raised in #8721, some more information is in #8723 and both of these link to some other related tickets. Comments welcome. Ideas welcome. Experiments welcome.

phadej · 2023-02-01T22:30:01Z

@Mikolaj the main issue is #8719

Mikolaj · 2023-02-01T22:36:13Z

#8719 is the release blocker and we hope it will fall out of #8721, which is the main task at hand, if the current investigation is accurate. If it's not solved by #8721, we have to re-asses. Anyway, everything is linked from the two tickets I mentioned.

phadej · 2023-02-01T22:40:26Z

#8721 won't solve #8719. The plan.json change is only masking the breaking changes done elsewhere. The proper fix is #8723, but it essentially reverts what this patch did.

phadej · 2023-02-01T22:43:22Z

To be clear: I don't think you can have this patch and fix #8719 at the same time. local tarballs should not be affected by flags meant for local packages.

The fact v2-install --lib is done how it is is unfortunate.

There should be a way to explicitly tell that "when you install/build/.. package foo apply these flags to package foo", that would be generally useful too. (And we can do that in cabal.project, I guess - and maybe the that can be done for fake project the v2-install --lib builds)

Mikolaj · 2023-02-01T22:46:13Z

OK, I stand corrected.

gbaz · 2023-02-02T02:18:59Z

After examining this for a while I'm coming around to the point of view of @phadej

A way to see the issue is that I think that even with this PR, cabal install foo-pkg --disable-library-vanilla will ignore that flag if run on a package foo-pkg in a remote repo. Before that pr, this would fail also if run inside a cabal project. After this pr, if its run inside a project directory, it will apply the flag, but if its run on a remote package, it won't. That seems inconsistent. The design, for better or worse, is that cabal never lets you apply flags when running a v2-install. I don't think this fixes that underlying issue, it just confuses things further.

I think the change proposed in #8723 is the most straightforward (but I don't understand the comment about it breaking cabal install --lib ??). We would couple that with flipping the test.

I believe this PR actually packed in another change, which is applying the static and dynamic flags to the setup configs as well, and I think that should stick around. (That was actually the basis on which I had supported this PR to begin with, not realizing it packed in this other thing too!). So that's why I'd propose selective flipping that one line while keeping other refactors (which also seem generally useful), as opposed to a wholesale revert.

Mikolaj · 2023-02-07T15:06:40Z

@bairyn: I haven't heard from you and we have to wrap up for the 3.10 (pre-)release. Are you OK with me reverting this PR wholesale on 3.10 and master branches and we can then slowly work our way towards salvaging the clarification and simplification ideas that are contained in it, using the @phadej input? Would that break any of your existing work, e.g., on the other PRs (I hope not, given they compile and test fine on just the bare master branch)?

This reverts commit b547ead from haskell#8623. Unexpected side-effect has been found, so these code improvements have to be done differently. The other commit in the PR is a test and it's retained.

Mikolaj · 2023-02-08T17:42:32Z

The (half-)reversion I'd propose is in #8744.

This reverts commit b547ead from haskell#8623. Unexpected side-effect has been found, so these code improvements have to be done differently. The other commit in the PR is a test and it's retained.

This reverts commit b547ead from #8623. Unexpected side-effect has been found, so these code improvements have to be done differently. The other commit in the PR is a test and it's retained. (cherry picked from commit 533cbc1)

bairyn mentioned this pull request Dec 3, 2022

Track static vs. dynamic dependencies. #8461

Closed

Mikolaj added type: enhancement re: dynamic-linking Concerning dynamic linking (e.g. flags "shared", "*-dynamic") labels Dec 4, 2022

bairyn force-pushed the fix-project-local-flags branch from cf42086 to a087092 Compare December 10, 2022 02:06

gbaz approved these changes Dec 10, 2022

View reviewed changes

bairyn force-pushed the fix-project-local-flags branch from a087092 to e38e2f8 Compare December 22, 2022 07:02

bairyn force-pushed the fix-project-local-flags branch from e38e2f8 to 2334bcb Compare December 22, 2022 07:12

Add a ‘NonignoredConfigs’ test that fails without our fix.

867cbb9

bairyn force-pushed the fix-project-local-flags branch from 2334bcb to 867cbb9 Compare December 23, 2022 00:45

Mikolaj approved these changes Dec 24, 2022

View reviewed changes

bairyn added the merge me Tell Mergify Bot to merge label Dec 24, 2022

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Dec 26, 2022

mergify bot merged commit 13e704d into haskell:master Dec 26, 2022

phadej mentioned this pull request Feb 1, 2023

Fix #8719. Local tarballs are not local packages. #8723

Closed

Mikolaj mentioned this pull request Feb 8, 2023

Revert "Fix project-local build flags being ignored." #8744

Merged

gbaz mentioned this pull request Feb 23, 2023

Apply command line flags to install packages (#8637) #8779

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix project-local flags being ignored #8623

Fix project-local flags being ignored #8623

bairyn commented Dec 3, 2022

bairyn commented Dec 10, 2022 •

edited

Loading

gbaz commented Dec 10, 2022

gbaz commented Dec 10, 2022

bairyn commented Dec 22, 2022

Mikolaj commented Dec 22, 2022

bairyn commented Dec 23, 2022

Mikolaj left a comment

Mikolaj commented Feb 1, 2023

phadej commented Feb 1, 2023

Mikolaj commented Feb 1, 2023 •

edited

Loading

phadej commented Feb 1, 2023 •

edited

Loading

phadej commented Feb 1, 2023 •

edited

Loading

Mikolaj commented Feb 1, 2023

gbaz commented Feb 2, 2023

Mikolaj commented Feb 7, 2023

Mikolaj commented Feb 8, 2023

Fix project-local flags being ignored #8623

Fix project-local flags being ignored #8623

Conversation

bairyn commented Dec 3, 2022

bairyn commented Dec 10, 2022 • edited Loading

gbaz commented Dec 10, 2022

gbaz commented Dec 10, 2022

bairyn commented Dec 22, 2022

Mikolaj commented Dec 22, 2022

bairyn commented Dec 23, 2022

Mikolaj left a comment

Choose a reason for hiding this comment

Mikolaj commented Feb 1, 2023

phadej commented Feb 1, 2023

Mikolaj commented Feb 1, 2023 • edited Loading

phadej commented Feb 1, 2023 • edited Loading

phadej commented Feb 1, 2023 • edited Loading

Mikolaj commented Feb 1, 2023

gbaz commented Feb 2, 2023

Mikolaj commented Feb 7, 2023

Mikolaj commented Feb 8, 2023

bairyn commented Dec 10, 2022 •

edited

Loading

Mikolaj commented Feb 1, 2023 •

edited

Loading

phadej commented Feb 1, 2023 •

edited

Loading

phadej commented Feb 1, 2023 •

edited

Loading